Ehancement: Named AgentSet + flexible lookup & stricter separation of concerns
#172
Conversation
…ed agent set management
…oved performance and clarity
…roved clarity and performance
…od for AgentSetsAccessor
…ement and enhance rename method for better delegation to AgentsDF.
…messages for key lookups
…ames in ValueError for better debugging
…gent sets as a list for multiple matches and improve error messaging for better clarity.
…ct functionality and error handling
…o 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…ency and clarity in the abstract class naming.
…proved consistency and clarity; enhance error messaging in __getitem__ and rename methods for better readability.
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #172 +/- ##
==========================================
- Coverage 92.54% 89.23% -3.31%
==========================================
Files 14 14
Lines 1717 2007 +290
==========================================
+ Hits 1589 1791 +202
- Misses 128 216 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…nt set types, improving performance and clarity in KeyError messages.
…arity in AGENTS.md, agents.py, agentset.py, and test_sets_accessor.py; improve formatting in test cases.
…r-agentsets-instead-of-list-in-agentsdf' of https://github.com/projectmesa/mesa-frames into 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…cessor; update parameter descriptions for improved understanding.
…update conflict resolution and mode descriptions for improved understanding.
…type annotations for parameters.
…cessor and AgentSetsAccessor; update default values and descriptions for parameters.
…multiple files feat: add seed property to Model for better random generator management docs: update docstrings in AbstractDataCollector for consistency and clarity
for more information, see https://pre-commit.ci
…on build artifacts
…tructure-for-agentsets-instead-of-list-in-agentsdf
…port cycles feat: add typing-extensions dependency for enhanced type support
…r-agentsets-instead-of-list-in-agentsdf' of https://github.com/projectmesa/mesa-frames into 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…as to clarify usage and avoid import cycles
…tructure-for-agentsets-instead-of-list-in-agentsdf
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mesa_frames/concrete/model.py (1)
97-100: Duplicatestepsproperty definition — remove one.The
stepsproperty is defined twice: here (lines 97–100) and again at lines 154–163. The second definition will silently override this one. Remove the duplicate.- @property - def steps(self) -> int: - """Get the current step count.""" - return self._steps - def reset_randomizer(self, seed: int | Sequence[int] | None) -> None:Keep the more complete definition at lines 154–163.
♻️ Duplicate comments (1)
mesa_frames/concrete/datacollector.py (1)
555-556: Same bareExceptioncatch as above.Apply the same fix here when extracting the helper.
🧹 Nitpick comments (7)
mesa_frames/concrete/datacollector.py (2)
181-189: Duplicated helper function — consider extracting to module level.
_is_str_collectionis defined identically here and again at lines 548–556. Extract it once at module scope to reduce duplication.+def _is_str_collection(x: Any) -> bool: + """Return True if x is a non-string collection of strings.""" + if isinstance(x, str): + return False + try: + from collections.abc import Collection + return isinstance(x, Collection) and all(isinstance(i, str) for i in x) + except TypeError: + return False + + class DataCollector(AbstractDataCollector):Then remove the nested definitions at lines 181–189 and 548–556.
188-189: Narrow exception catch toTypeError.The
all(isinstance(...))call would raiseTypeErrorif iteration fails. Catching bareExceptionmasks unrelated bugs.- except Exception: - return False + except TypeError: + return Falsemesa_frames/abstract/agentset.py (1)
51-51: Annotate mutable class attribute withClassVar.
_copy_only_referenceis a mutable list at class level. Mark it withClassVarto signal it's not an instance attribute.+from typing import ClassVar + class AbstractAgentSet(CopyMixin, DataFrameMixin): - _copy_only_reference: list[str] = ["_model"] + _copy_only_reference: ClassVar[list[str]] = ["_model"]mesa_frames/concrete/agentset.py (1)
588-592: Redundantnamecheck in__getattr__.The
if key == "name"check on lines 589-590 is unnecessary. Sincenameis defined as a property (lines 674-677), Python's attribute resolution will find the property before falling through to__getattr__. This branch will never execute.Apply this diff to remove the dead branch:
def __getattr__(self, key: str) -> Any: - if key == "name": - return self.name super().__getattr__(key) return self._df[key]mesa_frames/concrete/agentsetregistry.py (2)
149-159: Unused variablesingle.The variable
singleis assigned at lines 153 and 156 but never used. This appears to be leftover from removed functionality.Apply this diff to remove the unused variable:
if isinstance(target, (AgentSet, str)): if new_name is None: raise TypeError("new_name must be provided for single rename") pairs_idx: list[tuple[int, str]] = [(_resolve_one(target), new_name)] - single = True elif isinstance(target, dict): pairs_idx = [(_resolve_one(k), v) for k, v in target.items()] - single = False else: pairs_idx = [(_resolve_one(k), v) for k, v in target] - single = False
244-256: Add exception chaining for clearer error context.When re-raising
KeyErroras a newKeyError, addingfrom err(orfrom None) makes the traceback clearer by showing the relationship between exceptions.Apply this diff:
def _find_index_by_name(name: str) -> int: try: return name_to_idx[name] except KeyError: - raise KeyError(f"Agent set '{name}' not found") + raise KeyError(f"Agent set '{name}' not found") from None else: def _find_index_by_name(name: str) -> int: for i, s in enumerate(obj._agentsets): if s.name == name: return i - raise KeyError(f"Agent set '{name}' not found") + raise KeyError(f"Agent set '{name}' not found") from Nonemesa_frames/abstract/agentsetregistry.py (1)
584-626: Note:keys()anditems()silently skip sets withNonenames.When using
key_by="name", sets withname=Noneare silently excluded from the iteration (lines 598-599 and 621-622). This could lead to unexpected behavior if some AgentSets haveNonenames. Consider documenting this behavior in the docstrings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
examples/sugarscape_ig/ss_polars/agents.py(3 hunks)mesa_frames/abstract/agentset.py(12 hunks)mesa_frames/abstract/agentsetregistry.py(11 hunks)mesa_frames/abstract/datacollector.py(6 hunks)mesa_frames/concrete/agentset.py(6 hunks)mesa_frames/concrete/agentsetregistry.py(6 hunks)mesa_frames/concrete/datacollector.py(3 hunks)mesa_frames/concrete/model.py(3 hunks)mesa_frames/types_.py(2 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mesa_frames/abstract/datacollector.py
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-04-29T09:25:34.183Z
Learnt from: adamamer20
Repo: projectmesa/mesa-frames PR: 143
File: mesa_frames/abstract/space.py:50-63
Timestamp: 2025-04-29T09:25:34.183Z
Learning: The project mesa-frames has been upgraded to Python 3.11, which provides native support for `Self` type in the standard typing module, eliminating the need for imports from typing_extensions.
Applied to files:
pyproject.tomlmesa_frames/concrete/agentset.pymesa_frames/types_.py
📚 Learning: 2025-09-19T15:01:35.063Z
Learnt from: adamamer20
Repo: projectmesa/mesa-frames PR: 172
File: mesa_frames/abstract/agentset.py:460-470
Timestamp: 2025-09-19T15:01:35.063Z
Learning: In mesa-frames codebase, avoid using TYPE_CHECKING guards for type annotations because the project uses beartype for runtime type checking, which requires the actual type objects to be available at runtime. TYPE_CHECKING guards would hide these imports from runtime execution, breaking beartype compatibility.
Applied to files:
mesa_frames/abstract/agentset.pymesa_frames/concrete/agentset.pymesa_frames/types_.pymesa_frames/abstract/agentsetregistry.py
📚 Learning: 2025-04-29T09:20:40.575Z
Learnt from: adamamer20
Repo: projectmesa/mesa-frames PR: 143
File: mesa_frames/concrete/agentset.py:88-88
Timestamp: 2025-04-29T09:20:40.575Z
Learning: When using `from __future__ import annotations` at the top of a file, forward references in type annotations do not need to be enclosed in strings, even when using runtime type checking with beartype. Tools like pyupgrade deliberately remove such quoted annotations. Ruff may still generate F821 warnings for unquoted imports, but these can be safely ignored.
Applied to files:
mesa_frames/abstract/agentset.pymesa_frames/concrete/agentset.pymesa_frames/abstract/agentsetregistry.py
📚 Learning: 2025-04-29T09:23:26.204Z
Learnt from: adamamer20
Repo: projectmesa/mesa-frames PR: 143
File: mesa_frames/abstract/mixin.py:407-407
Timestamp: 2025-04-29T09:23:26.204Z
Learning: In the mesa-frames project, overload type annotations in abstract classes are used to define multiple method signatures for static type checking purposes, but they don't affect runtime behavior. Method implementations in concrete classes determine the actual behavior and default values.
Applied to files:
mesa_frames/types_.py
🧬 Code graph analysis (2)
mesa_frames/concrete/agentset.py (3)
mesa_frames/abstract/agentset.py (4)
model(523-531)name(512-520)rename(556-577)index(475-483)mesa_frames/concrete/agentsetregistry.py (1)
rename(116-219)mesa_frames/abstract/mixin.py (1)
_get_obj(139-155)
mesa_frames/concrete/agentsetregistry.py (2)
mesa_frames/abstract/agentsetregistry.py (13)
AbstractAgentSetRegistry(64-668)get(285-287)get(291-293)get(297-301)get(305-315)get(318-329)contains(169-176)contains(180-187)contains(190-203)do(207-216)do(220-234)do(237-281)ids(660-668)mesa_frames/abstract/mixin.py (2)
_get_obj(139-155)copy(73-137)
🪛 Ruff (0.14.6)
mesa_frames/abstract/agentset.py
51-51: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
523-523: Undefined name mesa_frames
(F821)
545-545: Undefined name mesa_frames
(F821)
mesa_frames/concrete/agentset.py
86-86: Undefined name mesa_frames
(F821)
319-319: Avoid specifying long messages outside the exception class
(TRY003)
mesa_frames/types_.py
104-108: Undefined name "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | " "str | Collection[" "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | str] | None"
(F821)
104-108: Undefined name "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | " "str | Collection[" "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | str] | None"
(F821)
104-108: Undefined name "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | " "str | Collection[" "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | str] | None"
(F821)
104-108: Undefined name "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | " "str | Collection[" "mesa_frames.abstract.agentset.AbstractAgentSet | " "type[mesa_frames.abstract.agentset.AbstractAgentSet] | str] | None"
(F821)
115-119: Undefined name "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | " "str | Collection[" "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | str] | None"
(F821)
115-119: Undefined name "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | " "str | Collection[" "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | str] | None"
(F821)
115-119: Undefined name "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | " "str | Collection[" "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | str] | None"
(F821)
115-119: Undefined name "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | " "str | Collection[" "mesa_frames.concrete.agentset.AgentSet | " "type[mesa_frames.concrete.agentset.AgentSet] | str] | None"
(F821)
123-137: __all__ is not sorted
Apply an isort-style sorting to __all__
(RUF022)
mesa_frames/concrete/datacollector.py
188-188: Do not catch blind exception: Exception
(BLE001)
239-241: Abstract raise to an inner function
(TRY301)
239-241: Avoid specifying long messages outside the exception class
(TRY003)
258-260: Avoid specifying long messages outside the exception class
(TRY003)
264-266: Avoid specifying long messages outside the exception class
(TRY003)
555-555: Do not catch blind exception: Exception
(BLE001)
mesa_frames/abstract/agentsetregistry.py
102-102: Undefined name mesa_frames
(F821)
104-104: Undefined name mesa_frames
(F821)
105-105: Undefined name mesa_frames
(F821)
146-146: Undefined name mesa_frames
(F821)
147-147: Undefined name mesa_frames
(F821)
172-172: Undefined name mesa_frames
(F821)
173-173: Undefined name mesa_frames
(F821)
183-183: Undefined name mesa_frames
(F821)
184-184: Undefined name mesa_frames
(F821)
233-233: Undefined name mesa_frames
(F821)
251-251: Undefined name mesa_frames
(F821)
287-287: Undefined name mesa_frames
(F821)
293-293: Undefined name mesa_frames
(F821)
299-299: Undefined name mesa_frames
(F821)
301-301: Undefined name mesa_frames
(F821)
307-307: Undefined name mesa_frames
(F821)
308-308: Undefined name mesa_frames
(F821)
309-309: Undefined name mesa_frames
(F821)
312-312: Undefined name mesa_frames
(F821)
313-313: Undefined name mesa_frames
(F821)
320-320: Undefined name mesa_frames
(F821)
321-321: Undefined name mesa_frames
(F821)
322-322: Undefined name mesa_frames
(F821)
325-325: Undefined name mesa_frames
(F821)
326-326: Undefined name mesa_frames
(F821)
357-357: Undefined name mesa_frames
(F821)
358-358: Undefined name mesa_frames
(F821)
428-428: Undefined name mesa_frames
(F821)
429-429: Undefined name mesa_frames
(F821)
435-435: Undefined name mesa_frames
(F821)
443-443: Undefined name mesa_frames
(F821)
448-448: Undefined name mesa_frames
(F821)
452-452: Undefined name mesa_frames
(F821)
453-453: Undefined name mesa_frames
(F821)
456-456: Undefined name mesa_frames
(F821)
458-458: Undefined name mesa_frames
(F821)
459-459: Undefined name mesa_frames
(F821)
466-466: Undefined name mesa_frames
(F821)
467-467: Undefined name mesa_frames
(F821)
487-487: Undefined name mesa_frames
(F821)
488-488: Undefined name mesa_frames
(F821)
508-508: Undefined name mesa_frames
(F821)
509-509: Undefined name mesa_frames
(F821)
529-529: Undefined name mesa_frames
(F821)
540-540: Avoid specifying long messages outside the exception class
(TRY003)
553-553: Avoid specifying long messages outside the exception class
(TRY003)
560-560: Undefined name mesa_frames
(F821)
586-586: Undefined name mesa_frames
(F821)
596-596: Avoid specifying long messages outside the exception class
(TRY003)
605-605: Undefined name mesa_frames
(F821)
606-606: Undefined name mesa_frames
(F821)
619-619: Avoid specifying long messages outside the exception class
(TRY003)
624-624: Undefined name mesa_frames
(F821)
mesa_frames/concrete/agentsetregistry.py
88-90: Avoid specifying long messages outside the exception class
(TRY003)
142-142: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
159-159: Local variable single is assigned to but never used
Remove assignment to unused variable single
(F841)
191-191: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
248-248: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
248-248: Avoid specifying long messages outside the exception class
(TRY003)
256-256: Avoid specifying long messages outside the exception class
(TRY003)
265-265: Avoid specifying long messages outside the exception class
(TRY003)
267-269: Avoid specifying long messages outside the exception class
(TRY003)
275-277: Avoid specifying long messages outside the exception class
(TRY003)
281-283: Avoid specifying long messages outside the exception class
(TRY003)
291-293: Avoid specifying long messages outside the exception class
(TRY003)
304-306: Avoid specifying long messages outside the exception class
(TRY003)
309-309: Avoid specifying long messages outside the exception class
(TRY003)
385-385: Avoid specifying long messages outside the exception class
(TRY003)
438-440: Avoid specifying long messages outside the exception class
(TRY003)
449-452: Avoid specifying long messages outside the exception class
(TRY003)
467-469: Avoid specifying long messages outside the exception class
(TRY003)
650-650: Avoid specifying long messages outside the exception class
(TRY003)
692-694: Avoid specifying long messages outside the exception class
(TRY003)
735-735: Avoid specifying long messages outside the exception class
(TRY003)
738-738: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (23)
examples/sugarscape_ig/ss_polars/agents.py (2)
37-46:eatcorrectly scopes sugar updates to this AgentSet; double‑check ID alignment andis_insemanticsFiltering
self.space.cellsdown to rows whoseagent_idis inself.index.implode()is a good fix to avoid cross‑set sugar mutations — only agents belonging to this set now get their sugar updated and later discarded based onsugar <= 0.Two points to verify:
- That
self.space.cells["agent_id"]uses the same ID space asself.index(e.g. the sameunique_idfield, not row offsets), otherwise some agents could be skipped or mis‑included.- That
cells["agent_id"].is_in(self.index.implode())behaves as intended with your current Polars version; usingimplode()here produces a list‑typed Series literal, which is a bit non‑obvious. If the goal is simple membership against all indices, you may want to confirm via a small test (or consider a more explicit pattern like building a Pythonsetor using a plain Series if supported).
203-205: Confirm.implode()usage inis_inforbest_moves["agent_id_center"]The logic in
get_best_movesstill matches the intent described in the comments (agents can move when their blocking agent has already moved, and then we drop agents that now have a best move). The new checks
- Line 203–205:
pl.col("blocking_agent_id").is_in(best_moves["agent_id_center"].implode())- Line 215–217:
pl.col("agent_id_center").is_in(best_moves["agent_id_center"].implode())depend on how Polars
Expr.is_ininteracts with a list‑typed Series produced by.implode().To avoid subtle bugs, especially as Polars evolves, it would be good to:
- Explicitly verify (with a focused test) that these
is_incalls behave exactly as “membership in the set of all currentagent_id_centervalues” whenbest_moveshas multiple rows.- Consider adding a short comment near the first use explaining that
best_moves["agent_id_center"].implode()is intentionally used as a list literal for membership, not as a per‑row list column, so future readers don’t “simplify” it back to the old expression.Also applies to: 215-217
pyproject.toml (1)
41-41: LGTM! Dependency needed for TypeAliasType compatibility.
typing-extensions>=4.15.0correctly providesTypeAliasTypefor Python 3.11 (where it's not yet in the stdlib). This enables the lazy type alias pattern used inmesa_frames/types_.py.mesa_frames/concrete/datacollector.py (1)
216-261: LGTM! Callable reporter handling with proper fallback.The registry-level → set-level fallback pattern is well-structured. The
TypeErrorcatch (line 243) correctly handles signature mismatches without swallowing other exceptions.mesa_frames/types_.py (2)
123-137: LGTM! Public API surface correctly exported.The selectors and common type aliases are properly exposed. The static analysis warning about sorting is a style preference.
101-121: TypeAliasType string lazy resolution is correctly implemented and verified to work with beartype.The code uses
TypeAliasTypewith string-based forward references (lines 101–121 inmesa_frames/types_.py) to avoid circular import cycles. This pattern is:
Supported:
typing-extensions>=4.15.0(specified inpyproject.toml) provides fullTypeAliasTypeandevaluate_forward_refsupport for lazy string resolution.Beartype-compatible: beartype's runtime type-checking pipeline resolves stringified type aliases at validation time. The codebase confirms this works—beartype is integrated via
beartype_this_package()in the dev environment (MESA_FRAMES_RUNTIME_TYPECHECKING = "true"), and theAgentSetSelectorandAbstractAgentSetSelectoraliases are actively used in function signatures throughout the codebase without reported failures.Documented and intentional: The comment block (lines 93–99) correctly explains the strategy. The full module paths in strings (
"mesa_frames.abstract.agentset.AbstractAgentSet", etc.) are necessary to allow lazy resolution without eager imports.No issues found.
mesa_frames/abstract/agentset.py (3)
126-128: LGTM! Clean discard semantics usingsuppress.Using
contextlib.suppressfor the "remove if exists" pattern is idiomatic and avoids nested try/except.
365-371: LGTM! Subtraction operators correctly usediscardfor safe removal.Using
discardensures that removing non-existent agents doesn't raise errors, which aligns with standard set semantics.
522-553: LGTM! Model context properties well-designed.The
model,random, andspaceproperties provide clean access to the parent model's resources. The F821 warnings are expected due to beartype runtime requirements (per learnings).mesa_frames/concrete/model.py (3)
89-95: LGTM! Step-counting wrapper pattern.The wrapper captures the user-defined
step()and auto-increments_steps. This works correctly because subclass overrides exist beforesuper().__init__()completes.
116-136: LGTM! Seed property with proper delegation.The setter correctly delegates to
reset_randomizer, maintaining a single source of truth for RNG initialization.
146-152: LGTM! Step delegation through registry.Using
self.sets.do("step")aligns with the registry-first design. This correctly propagatesstep()to all containedAgentSetinstances.mesa_frames/concrete/agentset.py (3)
85-105: LGTM on constructor updates.The initialization properly sets up the
_nameattribute with a sensible default (class name) when not provided. The docstring accurately documents the newnameparameter.Based on learnings, the F821 warning for
mesa_frameson line 86 is expected and can be safely ignored due to beartype runtime type checking requirements.
256-290: LGTM ondo()method implementation.The masking logic correctly handles both full-mask (direct operation) and partial-mask (copy-operate-concatenate) cases. The inplace attribute copying at lines 283-286 properly propagates state changes back to
selfwhen operating on a masked subset.
309-321: LGTM onremove()method.The implementation correctly validates that all specified agents exist before removal and provides a clear error message when they don't. The early return for empty/None inputs is a good optimization.
mesa_frames/concrete/agentsetregistry.py (6)
80-114: LGTM onadd()method with unique name handling.The implementation correctly ensures unique names across the registry by auto-incrementing suffixes when conflicts occur. The ID uniqueness validation and proper extension of the agentsets list are well-implemented.
411-482: LGTM ondo()method implementation.The method correctly handles registry indices (lines 427, 436, 465), guards against duplicate type keys when
key_by='type'(lines 448-452), and properly preserves untouched sets when operating on a subset (lines 479-480). This addresses the issues flagged in previous reviews.
340-385: LGTM oncontains()method.The implementation provides efficient fast paths for single-value lookups and properly handles mixed iterable inputs. The early returns for empty iterables and the type-based dispatch are well-structured.
626-658: LGTM on_resolve_selector()helper.The method correctly handles all selector types (None, instance, type, name, and collections) with proper deduplication that preserves insertion order. The TypeError for unsupported element types provides good error feedback.
611-624: LGTM on_recompute_ids()helper.The method correctly rebuilds the registry-level
unique_idcache from the current AgentSets, handling both empty and non-empty states appropriately.
718-738: LGTM on__getitem__()implementation.The method correctly handles all three access patterns (by index, name, and type) with appropriate error handling and return types matching the overload signatures.
mesa_frames/abstract/agentsetregistry.py (2)
526-553: LGTM on__setitem__()implementation.The method correctly handles both index and name-based assignment, properly validates model ownership, and delegates to
replace()for existing entries oradd()for new name-based entries. The rename before add ensures the new set has the correct name.
658-667: LGTM onidsproperty definition.The abstract property provides a clear contract for accessing all agent unique_id values across contained sets.
|
@adamamer20 I also imported mesa_frames where it is being used. |
Ben-geo
left a comment
Ben-geo
left a comment
There was a problem hiding this comment.
LGTM! - Other than the first coderabbit suggestion!
…tructure-for-agentsets-instead-of-list-in-agentsdf
…r-agentsets-instead-of-list-in-agentsdf' of https://github.com/projectmesa/mesa-frames into 146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
…of a list in agentsdf
for more information, see https://pre-commit.ci
adamamer20
deleted the
146-enhancement-consider-using-a-key-based-structure-for-agentsets-instead-of-list-in-agentsdf
branch
This PR simplifies the mental model and usage of
AgentSet/AgentSetRegistryby:pl.DataFrame.This yields a friendlier, clearer and safer API for everyday tasks (query one set, broadcast across sets).
What changed
1) Registry scope
AgentSetRegistry.dfreturned a mapping{AgentSet → pl.DataFrame}and high-level helpers likegetoften operated directly on the sets’ underlying DataFrames, which was easy to misread as “registry-wide” operations.dfinterface anymore, nor any “agents” iterator. You always work per set (viamodel.sets[...]) and then use that set’s API (.df,.get,.set, …). The registry orchestrates and routes, but never acts like a DataFrame holder.Before → After: accessing DataFrames
Get one set’s frame
Iterate multiple sets’ frames
2) Named sets & flexible lookup
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.